Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

agetpass(): Allocate on the stack (alloca(3)) #1191

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

agetpass(): Allocate on the stack (alloca(3)) #1191

wants to merge 13 commits into from

Conversation

alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Jan 19, 2025 •
edited
Loading

Hi!

This is rather sensitive, and I'd like to have as many eyes as possible look at this code.

Cc: @hallyn , @ikerexxe , @stoeckmann , @thalman , @thesamesam , @ferivoz , @jubalh

Reasons for all this change:

  • I want to use these APIs more, to replace manual copying of passwords (array variable, STRTCPY(), and MEMZERO()). By using agetpass() everywhere, we get implicit and correct sizes everywhere, no truncation, the compiler enforces that we clear the password, and all the other goods from this API. However, that would increase the exposure of these passwords in the heap, which I'm not comfortable with.
    See Clear plaintext passwords in more error cases #1190 (comment).

Revisions:

v2
  • Remove unused include.
$ git range-diff master gh/agetpass agetpass 
1:  1059f4dc = 1:  1059f4dc lib/agetpass.*: Make these functions inline
2:  de2294b2 = 2:  de2294b2 lib/agetpass.h: agetpass_stdin(): Add missing attribute
3:  98069160 = 3:  98069160 lib/agetpass.h: Replace documentation by one-line comment
4:  55c4128f = 4:  55c4128f lib/agetpass.h: Move attribute to agetpass_internal()
5:  5a7b0868 = 5:  5a7b0868 lib/agetpass.*: agetpass(), agetpass_stdin(): Re-implement as macros
6:  f7f02f7b = 6:  f7f02f7b lib/agetpass.*: Move allocation to helper macro
7:  c317712d ! 7:  eb720911 lib/agetpass.*: Use alloca(3) to minimize visibility of passwords
    @@ lib/agetpass.h
      #include <errno.h>
      #include <limits.h>
      #include <readpassphrase.h>
    -@@
    + #include <stddef.h>
    +-#include <stdlib.h>
      #include <string.h>
      
      #include "alloc/malloc.h"
8:  f6fe91a1 ! 8:  f6aeb0ab lib/: PASS_MAX: Move definition to where it's used
    @@ lib/agetpass.h
      #include <readpassphrase.h>
      #include <stddef.h>
     +#include <stdio.h>
    - #include <stdlib.h>
      #include <string.h>
      
    -@@
    + #include "alloc/malloc.h"
      #include "string/memset/memzero.h"
v2b
  • Add missing include.
$ git range-diff master gh/agetpass agetpass 
1:  1059f4dc ! 1:  5be29cc8 lib/agetpass.*: Make these functions inline
    @@ lib/agetpass.h
      
      #include <config.h>
      
    --#include "attr.h"
    --#include "defines.h"
     +#include <limits.h>
     +#include <readpassphrase.h>
     +#include <stdlib.h>
     +#include <string.h>
     +
     +#include "alloc/malloc.h"
    + #include "attr.h"
    +-#include "defines.h"
     +
     +#if WITH_LIBBSD == 0
     +#include "freezero.h"
2:  de2294b2 = 2:  0d2fa0d8 lib/agetpass.h: agetpass_stdin(): Add missing attribute
3:  98069160 = 3:  d62af8e3 lib/agetpass.h: Replace documentation by one-line comment
4:  55c4128f = 4:  5dad6078 lib/agetpass.h: Move attribute to agetpass_internal()
5:  5a7b0868 = 5:  d9a9ed13 lib/agetpass.*: agetpass(), agetpass_stdin(): Re-implement as macros
6:  f7f02f7b = 6:  213c6e4f lib/agetpass.*: Move allocation to helper macro
7:  eb720911 ! 7:  17560499 lib/agetpass.*: Use alloca(3) to minimize visibility of passwords
    @@ lib/agetpass.h
      #include <string.h>
      
      #include "alloc/malloc.h"
    + #include "attr.h"
     -
     -#if WITH_LIBBSD == 0
     -#include "freezero.h"
8:  f6aeb0ab ! 8:  b7c072bd lib/: PASS_MAX: Move definition to where it's used
    @@ lib/agetpass.h
      #include <string.h>
      
      #include "alloc/malloc.h"
    +@@
      #include "string/memset/memzero.h"
v3
  • Use array notation.
$ git range-diff master gh/agetpass agetpass 
 1:  5be29cc8 =  1:  5be29cc8 lib/agetpass.*: Make these functions inline
 2:  0d2fa0d8 =  2:  0d2fa0d8 lib/agetpass.h: agetpass_stdin(): Add missing attribute
 3:  d62af8e3 =  3:  d62af8e3 lib/agetpass.h: Replace documentation by one-line comment
 4:  5dad6078 =  4:  5dad6078 lib/agetpass.h: Move attribute to agetpass_internal()
 5:  d9a9ed13 =  5:  d9a9ed13 lib/agetpass.*: agetpass(), agetpass_stdin(): Re-implement as macros
 6:  213c6e4f =  6:  213c6e4f lib/agetpass.*: Move allocation to helper macro
 7:  17560499 =  7:  17560499 lib/agetpass.*: Use alloca(3) to minimize visibility of passwords
 8:  b7c072bd =  8:  b7c072bd lib/: PASS_MAX: Move definition to where it's used
 -:  -------- >  9:  45459469 lib/agetpass.*: erase_pass(): Specify array parameter size
v3b
  • Add blank line separating public APIs and internals.
$ git range-diff master gh/agetpass agetpass 
 1:  5be29cc8 =  1:  5be29cc8 lib/agetpass.*: Make these functions inline
 2:  0d2fa0d8 =  2:  0d2fa0d8 lib/agetpass.h: agetpass_stdin(): Add missing attribute
 3:  d62af8e3 =  3:  d62af8e3 lib/agetpass.h: Replace documentation by one-line comment
 4:  5dad6078 =  4:  5dad6078 lib/agetpass.h: Move attribute to agetpass_internal()
 5:  d9a9ed13 =  5:  d9a9ed13 lib/agetpass.*: agetpass(), agetpass_stdin(): Re-implement as macros
 6:  213c6e4f !  6:  b7018e69 lib/agetpass.*: Move allocation to helper macro
    @@ lib/agetpass.h
     +#define agetpass(prompt)  agetpass_(prompt, RPP_REQUIRE_TTY)
     +#define agetpass_stdin()  agetpass_(NULL, RPP_STDIN)
     +
    ++
     +#define agetpass_(...)    getpass_(MALLOC(PASS_MAX + 2, char), __VA_ARGS__)
      
      
 7:  17560499 !  7:  5ebda71e lib/agetpass.*: Use alloca(3) to minimize visibility of passwords
    @@ lib/agetpass.h
      
      
      // Similar to getpass(3), but free of its problems.
    - #define agetpass(prompt)  agetpass_(prompt, RPP_REQUIRE_TTY)
    +@@
      #define agetpass_stdin()  agetpass_(NULL, RPP_STDIN)
      
    + 
     -#define agetpass_(...)    getpass_(MALLOC(PASS_MAX + 2, char), __VA_ARGS__)
     +#define agetpass_(...)    getpass_(alloca(PASS_MAX + 2), __VA_ARGS__)
      
 8:  b7c072bd =  8:  3f662bcb lib/: PASS_MAX: Move definition to where it's used
 9:  45459469 =  9:  30b50b13 lib/agetpass.*: erase_pass(): Specify array parameter size
v4 
$ git range-diff master gh/agetpass agetpass 
 1:  5be29cc8 <  -:  -------- lib/agetpass.*: Make these functions inline
 2:  0d2fa0d8 <  -:  -------- lib/agetpass.h: agetpass_stdin(): Add missing attribute
 3:  d62af8e3 !  1:  49a50f15 lib/agetpass.h: Replace documentation by one-line comment
    @@ Commit message
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    - ## lib/agetpass.h ##
    + ## lib/agetpass.c ##
     @@
    - 
    - 
    - inline void erase_pass(char *pass);
    -+
    -+// Similar to getpass(3), but free of its problems.
    - ATTR_MALLOC(erase_pass)
    - inline char *agetpass(const char *prompt);
    - ATTR_MALLOC(erase_pass)
    -@@ lib/agetpass.h: inline char *agetpass_stdin();
    - inline char *agetpass_internal(const char *prompt, int flags);
    + #endif /* WITH_LIBBSD */
      
      
     -/*
    @@ lib/agetpass.h: inline char *agetpass_stdin();
     - */
     -
     -
    - inline char *
    + static char *
      agetpass_internal(const char *prompt, int flags)
      {
    +@@ lib/agetpass.c: fail:
    +   return NULL;
    + }
    + 
    ++// Similar to getpass(3), but free of its problems.
    + char *
    + agetpass(const char *prompt)
    + {
 4:  5dad6078 <  -:  -------- lib/agetpass.h: Move attribute to agetpass_internal()
 5:  d9a9ed13 <  -:  -------- lib/agetpass.*: agetpass(), agetpass_stdin(): Re-implement as macros
 6:  b7018e69 <  -:  -------- lib/agetpass.*: Move allocation to helper macro
 7:  5ebda71e <  -:  -------- lib/agetpass.*: Use alloca(3) to minimize visibility of passwords
 8:  3f662bcb <  -:  -------- lib/: PASS_MAX: Move definition to where it's used
 9:  30b50b13 <  -:  -------- lib/agetpass.*: erase_pass(): Specify array parameter size
 -:  -------- >  2:  8f152cbb lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 -:  -------- >  3:  659ec092 lib/pass/agetpass.*: Redefine APIs as macros
 -:  -------- >  4:  b9602899 lib/pass/: readpass(): Add function
 -:  -------- >  5:  09da08b8 lib/: PASS_MAX: Define constant where it's used
 -:  -------- >  6:  0063db7a lib/pass/: passzero(): Add function
 -:  -------- >  7:  d523f592 lib/pass/: Use passzero() instead of its pattern
 -:  -------- >  8:  2edcb43d lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
 -:  -------- >  9:  67b08384 lib/pass/: Add alloca(3)-based variants of these APIs
 -:  -------- > 10:  58bf3008 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
 -:  -------- > 11:  f0241919 lib/pass/: Remove malloc(3)-based APIs, as they're unused
v5
  • Move macro to separate header <pass/limits.h>. This breaks a circular include.
$ git range-diff master gh/agetpass agetpass 
 1:  49a50f15 =  1:  49a50f15 lib/agetpass.h: Replace documentation by one-line comment
 2:  8f152cbb =  2:  8f152cbb lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 3:  659ec092 =  3:  659ec092 lib/pass/agetpass.*: Redefine APIs as macros
 4:  b9602899 =  4:  b9602899 lib/pass/: readpass(): Add function
 5:  09da08b8 <  -:  -------- lib/: PASS_MAX: Define constant where it's used
 -:  -------- >  5:  2b381daf lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 6:  0063db7a !  6:  8889e1af lib/pass/: passzero(): Add function
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    -   pam_pass_non_interactive.c \
        pass/agetpass.c \
        pass/agetpass.h \
    +   pass/limits.h \
     +  pass/passzero.c \
     +  pass/passzero.h \
        pass/readpass.c \
    @@ lib/pass/passzero.c (new)
     +
     +#include <config.h>
     +
    ++#include "pass/passzero.h"
    ++
    ++#include "pass/limits.h"
     +#include "string/memset/memzero.h"
     +
     +
    @@ lib/pass/passzero.h (new)
     +
     +#include <config.h>
     +
    -+#include "pass/readpass.h"
    ++#include "pass/limits.h"
     +
     +
     +char *passzero(char pass[PASS_MAX + 2]);
 7:  d523f592 !  7:  f7158ea4 lib/pass/: Use passzero() instead of its pattern
    @@ lib/pass/agetpass.c: fail:
     
      ## lib/pass/readpass.h ##
     @@
    - #include <limits.h>
      
      #include "attr.h"
    + #include "pass/limits.h"
     -#include "string/memset/memzero.h"
     +#include "pass/passzero.h"
      
      
    - // There is also a limit in PAM (PAM_MAX_RESP_SIZE), currently set to 512.
    -@@
    - #endif
    - 
    - 
     -ATTR_MALLOC(memzero)
     +ATTR_MALLOC(passzero)
      char *readpass(const char *prompt, char pass[PASS_MAX + 2], int flags);
 8:  2edcb43d !  8:  2d8f2580 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    -   pam_pass_non_interactive.c \
        pass/agetpass.c \
        pass/agetpass.h \
    +   pass/limits.h \
     +  pass/areadpass.c \
     +  pass/areadpass.h \
        pass/passzero.c \
 9:  67b08384 !  9:  bcf5f221 lib/pass/: Add alloca(3)-based variants of these APIs
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    -   pass/agetpass.h \
    +   pass/limits.h \
        pass/areadpass.c \
        pass/areadpass.h \
     +  pass/getpassa.c \
10:  58bf3008 = 10:  5bdd8c81 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
11:  f0241919 ! 11:  2f183652 lib/pass/: Remove malloc(3)-based APIs, as they're unused
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        pam_pass_non_interactive.c \
     -  pass/agetpass.c \
     -  pass/agetpass.h \
    +   pass/limits.h \
     -  pass/areadpass.c \
     -  pass/areadpass.h \
        pass/getpassa.c \
v6
  • Call alloca(3) via passalloca() before entering a loop, and then reuse the storage. This prevents stack overflows. (Thanks to CodeQL for catching that.)
  • Reorder parameters of readpass(). This makes it less consistent with readpassphrase(3), but more consistent with the other APIs, and easier to use. Anyway, readpassphrase isn't very well designed, so don't try to follow it.
  • Use getpass2() instead of readpassa() as a helper for getpassa().
$ git range-diff master gh/agetpass agetpass 
 1:  49a50f15 =  1:  49a50f15 lib/agetpass.h: Replace documentation by one-line comment
 2:  8f152cbb =  2:  8f152cbb lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 3:  659ec092 =  3:  659ec092 lib/pass/agetpass.*: Redefine APIs as macros
 4:  b9602899 !  4:  f58dab9c lib/pass/: readpass(): Add function
    @@ lib/pass/agetpass.c: agetpass_internal(const char *prompt, int flags)
                return NULL;
      
     -  if (readpassphrase(prompt, pass, PASS_MAX + 2, flags) == NULL)
    -+  if (readpass(prompt, pass, flags) == NULL)
    ++  if (readpass(pass, prompt, flags) == NULL)
                goto fail;
      
     -  len = strlen(pass);
    @@ lib/pass/readpass.c (new)
     +
     +// readpassphrase(3), but detect truncation, and memzero() on error.
     +char *
    -+readpass(const char *prompt, char pass[PASS_MAX + 2], int flags)
    ++readpass(char pass[PASS_MAX + 2], const char *prompt, int flags)
     +{
     +  size_t  len;
     +
    @@ lib/pass/readpass.h (new)
     +
     +
     +ATTR_MALLOC(memzero)
    -+char *readpass(const char *prompt, char pass[PASS_MAX + 2], int flags);
    ++char *readpass(char pass[PASS_MAX + 2], const char *prompt, int flags);
     +
     +
     +#endif  // include guard
 5:  2b381daf =  5:  3c778845 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 6:  8889e1af =  6:  0d4cbc8e lib/pass/: passzero(): Add function
 7:  f7158ea4 !  7:  6f70f4b1 lib/pass/: Use passzero() instead of its pattern
    @@ lib/pass/readpass.h
      
     -ATTR_MALLOC(memzero)
     +ATTR_MALLOC(passzero)
    - char *readpass(const char *prompt, char pass[PASS_MAX + 2], int flags);
    + char *readpass(char pass[PASS_MAX + 2], const char *prompt, int flags);
      
      
 8:  2d8f2580 !  8:  5a1f2072 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
    @@ lib/pass/agetpass.c
     -  if (pass == NULL)
     -          return NULL;
     -
    --  if (readpass(prompt, pass, flags) == NULL)
    +-  if (readpass(pass, prompt, flags) == NULL)
     -          goto fail;
     -
     -  return pass;
    @@ lib/pass/areadpass.c (new)
     +  if (pass == NULL)
     +          return NULL;
     +
    -+  if (readpass(prompt, pass, flags) == NULL)
    ++  if (readpass(pass, prompt, flags) == NULL)
     +          goto fail;
     +
     +  return pass;
 9:  bcf5f221 <  -:  -------- lib/pass/: Add alloca(3)-based variants of these APIs
 -:  -------- >  9:  6f7f9ed7 lib/pass/: passalloca(): Add macro
 -:  -------- > 10:  817344ec lib/pass/: getpass2{,_stdin}(): Add macros
 -:  -------- > 11:  5be30de1 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
10:  5bdd8c81 = 12:  b59f7fbc lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
11:  2f183652 ! 13:  da7e927d lib/pass/: Remove malloc(3)-based APIs, as they're unused
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        pass/limits.h \
     -  pass/areadpass.c \
     -  pass/areadpass.h \
    +   pass/getpass2.c \
    +   pass/getpass2.h \
        pass/getpassa.c \
    -   pass/getpassa.h \
    -   pass/passzero.c \
     
      ## lib/pass/agetpass.c (deleted) ##
     @@
    @@ lib/pass/areadpass.c (deleted)
     -  if (pass == NULL)
     -          return NULL;
     -
    --  if (readpass(prompt, pass, flags) == NULL)
    +-  if (readpass(pass, prompt, flags) == NULL)
     -          goto fail;
     -
     -  return pass;
 -:  -------- > 14:  8962be83 src/sulogin.c: main(): Add local variable
 -:  -------- > 15:  57b8ac5e src/: Call passalloca() before a loop
v6b
  • Fix scope of variable.
$ git range-diff master gh/agetpass agetpass 
 1:  49a50f15 =  1:  49a50f15 lib/agetpass.h: Replace documentation by one-line comment
 2:  8f152cbb =  2:  8f152cbb lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 3:  659ec092 =  3:  659ec092 lib/pass/agetpass.*: Redefine APIs as macros
 4:  f58dab9c =  4:  f58dab9c lib/pass/: readpass(): Add function
 5:  3c778845 =  5:  3c778845 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 6:  0d4cbc8e =  6:  0d4cbc8e lib/pass/: passzero(): Add function
 7:  6f70f4b1 =  7:  6f70f4b1 lib/pass/: Use passzero() instead of its pattern
 8:  5a1f2072 =  8:  5a1f2072 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
 9:  6f7f9ed7 =  9:  6f7f9ed7 lib/pass/: passalloca(): Add macro
10:  817344ec = 10:  817344ec lib/pass/: getpass2{,_stdin}(): Add macros
11:  5be30de1 = 11:  5be30de1 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
12:  b59f7fbc = 12:  b59f7fbc lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
13:  da7e927d = 13:  da7e927d lib/pass/: Remove malloc(3)-based APIs, as they're unused
14:  8962be83 = 14:  8962be83 src/sulogin.c: main(): Add local variable
15:  57b8ac5e ! 15:  c7ea351f src/: Call passalloca() before a loop
    @@ src/sulogin.c
      #include "pass/passzero.h"
      #include "prototypes.h"
      #include "pwauth.h"
    +@@ src/sulogin.c: int
    + main(int argc, char *argv[])
    + {
    +   int            err = 0;
    ++  char           *pass;
    +   char           **envp = environ;
    +   TERMIO         termio;
    +   struct passwd  pwent = {};
     @@ src/sulogin.c: main(int argc, char *argv[])
        (void) signal (SIGALRM, catch_signals); /* exit if the timer expires */
        (void) alarm (ALARM);           /* only wait so long ... */
      
     +  pass = passalloca();
        do {                    /* repeatedly get login/password pairs */
    -           char        *pass;
    +-          char        *pass;
                const char  *prompt;
    + 
    +           if (pw_entry("root", &pwent) == -1) {   /* get entry from password file */
     @@ src/sulogin.c: main(int argc, char *argv[])
      "(or give root password for system maintenance):");
v6c
  • Fix order of parameters. [Thanks to CodeQL, which caught this bug.]
$ git range-diff master gh/agetpass agetpass 
 1:  49a50f15 =  1:  49a50f15 lib/agetpass.h: Replace documentation by one-line comment
 2:  8f152cbb =  2:  8f152cbb lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 3:  659ec092 =  3:  659ec092 lib/pass/agetpass.*: Redefine APIs as macros
 4:  f58dab9c =  4:  f58dab9c lib/pass/: readpass(): Add function
 5:  3c778845 =  5:  3c778845 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 6:  0d4cbc8e =  6:  0d4cbc8e lib/pass/: passzero(): Add function
 7:  6f70f4b1 =  7:  6f70f4b1 lib/pass/: Use passzero() instead of its pattern
 8:  5a1f2072 =  8:  5a1f2072 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
 9:  6f7f9ed7 =  9:  6f7f9ed7 lib/pass/: passalloca(): Add macro
10:  817344ec = 10:  817344ec lib/pass/: getpass2{,_stdin}(): Add macros
11:  5be30de1 ! 11:  00166e59 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
    @@ lib/pass/getpassa.h (new)
     +
     +
     +// Similar to getpass(3), but free of its problems, and using alloca(3).
    -+#define getpassa(prompt)  getpass2(prompt, passalloca())
    ++#define getpassa(prompt)  getpass2(passalloca(), prompt)
     +#define getpassa_stdin()  getpass2_stdin(passalloca())
     +
     +
12:  b59f7fbc = 12:  d129f6f9 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
13:  da7e927d = 13:  cd6c4bcb lib/pass/: Remove malloc(3)-based APIs, as they're unused
14:  8962be83 = 14:  9e11cfe4 src/sulogin.c: main(): Add local variable
15:  c7ea351f = 15:  0a5c77e2 src/: Call passalloca() before a loop
v7
  • Accept a NULL as input to passzero(). Make it a no-op, just like free(NULL). It's a useful thing to do.
    This fixes an accidental bug I had introduced earlier. In src/sulogin.c, I was passing a NULL to passzero().
    Code is also much simpler (and safer) when you can pass NULL to destructor APIs.
$ git range-diff master gh/agetpass agetpass 
 1:  49a50f15 =  1:  49a50f15 lib/agetpass.h: Replace documentation by one-line comment
 2:  8f152cbb =  2:  8f152cbb lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 3:  659ec092 =  3:  659ec092 lib/pass/agetpass.*: Redefine APIs as macros
 4:  f58dab9c =  4:  f58dab9c lib/pass/: readpass(): Add function
 5:  3c778845 =  5:  3c778845 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 6:  0d4cbc8e !  6:  b163084c lib/pass/: passzero(): Add function
    @@ lib/pass/passzero.c (new)
     +
     +#include "pass/passzero.h"
     +
    ++#include <stddef.h>
    ++
     +#include "pass/limits.h"
     +#include "string/memset/memzero.h"
     +
    @@ lib/pass/passzero.c (new)
     +char *
     +passzero(char pass[PASS_MAX + 2])
     +{
    ++  if (pass == NULL)
    ++          return NULL;
    ++
     +  return memzero(pass, PASS_MAX + 2);
     +}
     
 7:  6f70f4b1 !  7:  2db3c71f lib/pass/: Use passzero() instead of its pattern
    @@ lib/pass/agetpass.c: fail:
      erase_pass(char *pass)
      {
     -  freezero(pass, PASS_MAX + 2);
    -+  if (pass != NULL)
    -+          passzero(pass);
    -+  free(pass);
    ++  free(passzero(pass));
      }
     
      ## lib/pass/readpass.h ##
 8:  5a1f2072 !  8:  70171c15 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
    @@ lib/pass/agetpass.c
     -void
     -erase_pass(char *pass)
     -{
    --  if (pass != NULL)
    --          passzero(pass);
    --  free(pass);
    +-  free(passzero(pass));
     -}
     
      ## lib/pass/agetpass.h ##
    @@ lib/pass/areadpass.c (new)
     +void
     +erase_pass(char pass[PASS_MAX + 2])
     +{
    -+  if (pass != NULL)
    -+          passzero(pass);
    -+  free(pass);
    ++  free(passzero(pass));
     +}
     
      ## lib/pass/areadpass.h (new) ##
 9:  6f7f9ed7 =  9:  addd1e52 lib/pass/: passalloca(): Add macro
10:  817344ec = 10:  ef248eaa lib/pass/: getpass2{,_stdin}(): Add macros
11:  00166e59 = 11:  598a4137 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
12:  d129f6f9 = 12:  e9774933 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
13:  cd6c4bcb ! 13:  18c957d0 lib/pass/: Remove malloc(3)-based APIs, as they're unused
    @@ lib/pass/areadpass.c (deleted)
     -void
     -erase_pass(char pass[PASS_MAX + 2])
     -{
    --  if (pass != NULL)
    --          passzero(pass);
    --  free(pass);
    +-  free(passzero(pass));
     -}
     
      ## lib/pass/areadpass.h (deleted) ##
14:  9e11cfe4 = 14:  96fcab47 src/sulogin.c: main(): Add local variable
15:  0a5c77e2 = 15:  fb69c8d4 src/: Call passalloca() before a loop
v7b
  • Rebase
$ git range-diff master..gh/agetpass shadow/master..agetpass 
 1:  49a50f15 =  1:  a1287da5 lib/agetpass.h: Replace documentation by one-line comment
 2:  8f152cbb !  2:  e431d385 lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
    @@ src/gpasswd.c
      ## src/newgrp.c ##
     @@
      #include <stdio.h>
    - #include <assert.h>
    + #include <sys/types.h>
      
     -#include "agetpass.h"
      #include "alloc/x/xmalloc.h"
    @@ src/newgrp.c
      #include "getdef.h"
     +#include "pass/agetpass.h"
      #include "prototypes.h"
    - #include "shadowlog.h"
    - #include "string/sprintf/snprintf.h"
    + #include "search/l/lfind.h"
    + #include "search/l/lsearch.h"
     
      ## src/passwd.c ##
     @@
 3:  659ec092 =  3:  7c3a1b8c lib/pass/agetpass.*: Redefine APIs as macros
 4:  f58dab9c =  4:  a9b23bee lib/pass/: readpass(): Add function
 5:  3c778845 =  5:  90e48a18 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 6:  b163084c =  6:  707c6d20 lib/pass/: passzero(): Add function
 7:  2db3c71f =  7:  79cd525f lib/pass/: Use passzero() instead of its pattern
 8:  70171c15 =  8:  66ae8a37 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
 9:  addd1e52 =  9:  e1bafc01 lib/pass/: passalloca(): Add macro
10:  ef248eaa = 10:  b2858c80 lib/pass/: getpass2{,_stdin}(): Add macros
11:  598a4137 = 11:  b77318f9 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
12:  e9774933 ! 12:  ed2dcc70 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
    @@ src/newgrp.c
     +#include "pass/getpassa.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
    - #include "shadowlog.h"
    - #include "string/sprintf/snprintf.h"
    + #include "search/l/lfind.h"
    + #include "search/l/lsearch.h"
     @@ src/newgrp.c: static void check_perms (const struct group *grp,
                 * get the password from her, and set the salt for
                 * the decryption from the group file.
13:  18c957d0 = 13:  9ca1f28a lib/pass/: Remove malloc(3)-based APIs, as they're unused
14:  96fcab47 = 14:  2b98f9cf src/sulogin.c: main(): Add local variable
15:  fb69c8d4 = 15:  d6ceb227 src/: Call passalloca() before a loop
v7c
  • Reorder commits.
  • Document in the commit message that alloca(3) in a loop is bad.
$ git range-diff shadow/master gh/agetpass agetpass 
14:  2b98f9cf !  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
    @@ Metadata
      ## Commit message ##
         src/sulogin.c: main(): Add local variable
     
    -    This simplifies the getpassa() call into a single line.
    +    This simplifies the agetpass() call into a single line.
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    @@ src/sulogin.c: main(int argc, char *argv[])
                 */
     -
     -          /* get a password for root */
    --          pass = getpassa(_(
    +-          pass = agetpass (_(
     +          prompt = _(
      "\n"
      "Type control-d to proceed with normal startup,\n"
    @@ src/sulogin.c: main(int argc, char *argv[])
     +"(or give root password for system maintenance):");
     +
     +          /* get a password for root */
    -+          pass = getpassa(prompt);
    ++          pass = agetpass(prompt);
     +
                /*
                 * XXX - can't enter single user mode if root password is
 1:  a1287da5 =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 2:  e431d385 =  3:  bc41bf92 lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 3:  7c3a1b8c =  4:  1c4bb330 lib/pass/agetpass.*: Redefine APIs as macros
 4:  a9b23bee =  5:  3be94804 lib/pass/: readpass(): Add function
 5:  90e48a18 =  6:  5185ddc7 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 6:  707c6d20 =  7:  a75a8f83 lib/pass/: passzero(): Add function
 7:  79cd525f =  8:  345b50c5 lib/pass/: Use passzero() instead of its pattern
 8:  66ae8a37 =  9:  e18d2c84 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
 9:  e1bafc01 = 10:  5c1b03f1 lib/pass/: passalloca(): Add macro
10:  b2858c80 = 11:  f0212fb5 lib/pass/: getpass2{,_stdin}(): Add macros
11:  b77318f9 = 12:  1399edba lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
12:  ed2dcc70 ! 13:  0e0b08ae lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
    @@ Commit message
     
         Now all passwords live in the stack, and are never copied into the heap.
     
    +    This introduces a subtle issue: while it's fine to call malloc(3) in a
    +    loop, it is dangerous to call alloca(3) in a loop (since there's no way
    +    to free that memory).  The next commit will fix that.  I've addressed it
    +    in a separate commit, for readability.
    +
         Signed-off-by: Alejandro Colomar <[email protected]>
     
      ## lib/pwauth.c ##
    @@ src/sulogin.c
      #include "pwauth.h"
      /*@-exitarg@*/
     @@ src/sulogin.c: main(int argc, char *argv[])
    -            */
    + "(or give root password for system maintenance):");
      
                /* get a password for root */
    --          pass = agetpass (_(
    -+          pass = getpassa(_(
    - "\n"
    - "Type control-d to proceed with normal startup,\n"
    - "(or give root password for system maintenance):"));
    +-          pass = agetpass(prompt);
    ++          pass = getpassa(prompt);
    + 
    +           /*
    +            * XXX - can't enter single user mode if root password is
     @@ src/sulogin.c: main(int argc, char *argv[])
                 * --marekm
                 */
15:  d6ceb227 = 14:  0a8e8d1c src/: Call passalloca() before a loop
13:  9ca1f28a = 15:  9c494133 lib/pass/: Remove malloc(3)-based APIs, as they're unused
v8
  • Put getpassa() and getpass2() in the same header.
$ git range-diff master gh/agetpass agetpass 
 1:  b02a8eb2 =  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
 2:  5deb458d =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 3:  bc41bf92 =  3:  bc41bf92 lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 4:  1c4bb330 =  4:  1c4bb330 lib/pass/agetpass.*: Redefine APIs as macros
 5:  3be94804 =  5:  3be94804 lib/pass/: readpass(): Add function
 6:  5185ddc7 =  6:  5185ddc7 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 7:  a75a8f83 =  7:  a75a8f83 lib/pass/: passzero(): Add function
 8:  345b50c5 =  8:  345b50c5 lib/pass/: Use passzero() instead of its pattern
 9:  e18d2c84 =  9:  e18d2c84 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
10:  5c1b03f1 = 10:  5c1b03f1 lib/pass/: passalloca(): Add macro
11:  f0212fb5 = 11:  f0212fb5 lib/pass/: getpass2{,_stdin}(): Add macros
12:  1399edba <  -:  -------- lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
 -:  -------- > 12:  0562e2e5 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
13:  0e0b08ae ! 13:  e8d08f96 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
    @@ lib/pwauth.c
      
     -#include "pass/agetpass.h"
      #include "defines.h"
    -+#include "pass/getpassa.h"
    ++#include "pass/getpass2.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
      #include "pwauth.h"
    @@ src/gpasswd.c
      #include "groupio.h"
      #include "nscd.h"
     -#include "pass/agetpass.h"
    -+#include "pass/getpassa.h"
    ++#include "pass/getpass2.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
      #ifdef SHADOWGRP
    @@ src/newgrp.c
      #include "exitcodes.h"
      #include "getdef.h"
     -#include "pass/agetpass.h"
    -+#include "pass/getpassa.h"
    ++#include "pass/getpass2.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
      #include "search/l/lfind.h"
    @@ src/passwd.c
      #include "getdef.h"
      #include "nscd.h"
     -#include "pass/agetpass.h"
    -+#include "pass/getpassa.h"
    ++#include "pass/getpass2.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
      #include "pwauth.h"
    @@ src/sulogin.c
      #include "defines.h"
      #include "getdef.h"
     -#include "pass/agetpass.h"
    -+#include "pass/getpassa.h"
    ++#include "pass/getpass2.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
      #include "pwauth.h"
14:  0a8e8d1c ! 14:  8c4650c2 src/: Call passalloca() before a loop
    @@ Commit message
     
      ## src/gpasswd.c ##
     @@
    - #include "exitcodes.h"
      #include "groupio.h"
      #include "nscd.h"
    --#include "pass/getpassa.h"
    -+#include "pass/getpass2.h"
    + #include "pass/getpass2.h"
     +#include "pass/passalloca.h"
      #include "pass/passzero.h"
      #include "prototypes.h"
    @@ src/gpasswd.c: static void change_passwd (struct group *gr)
     
      ## src/passwd.c ##
     @@
    - #include "defines.h"
      #include "getdef.h"
      #include "nscd.h"
    -+#include "pass/getpass2.h"
    - #include "pass/getpassa.h"
    + #include "pass/getpass2.h"
     +#include "pass/passalloca.h"
      #include "pass/passzero.h"
      #include "prototypes.h"
    @@ src/passwd.c: static int new_password (const struct passwd *pw)
     
      ## src/sulogin.c ##
     @@
    - #include "attr.h"
      #include "defines.h"
      #include "getdef.h"
    --#include "pass/getpassa.h"
    -+#include "pass/getpass2.h"
    + #include "pass/getpass2.h"
     +#include "pass/passalloca.h"
      #include "pass/passzero.h"
      #include "prototypes.h"
15:  9c494133 ! 15:  1d7a2ead lib/pass/: Remove malloc(3)-based APIs, as they're unused
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
     -  pass/areadpass.h \
        pass/getpass2.c \
        pass/getpass2.h \
    -   pass/getpassa.c \
    +   pass/passalloca.c \
     
      ## lib/pass/agetpass.c (deleted) ##
     @@
v8b
  • White space
$ git range-diff master gh/agetpass agetpass 
 1:  b02a8eb2 =  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
 2:  5deb458d =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 3:  bc41bf92 =  3:  bc41bf92 lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 4:  1c4bb330 =  4:  1c4bb330 lib/pass/agetpass.*: Redefine APIs as macros
 5:  3be94804 =  5:  3be94804 lib/pass/: readpass(): Add function
 6:  5185ddc7 =  6:  5185ddc7 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 7:  a75a8f83 =  7:  a75a8f83 lib/pass/: passzero(): Add function
 8:  345b50c5 =  8:  345b50c5 lib/pass/: Use passzero() instead of its pattern
 9:  e18d2c84 =  9:  e18d2c84 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
10:  5c1b03f1 ! 10:  15bc4454 lib/pass/: passalloca(): Add macro
    @@ lib/pass/passalloca.h (new)
     +#include "pass/limits.h"
     +
     +
    -+#define passalloca()  alloca(PASS_MAX + 2)
    ++#define passalloca()   alloca(PASS_MAX + 2)
     +
     +
     +#endif  // include guard
11:  f0212fb5 = 11:  191532d9 lib/pass/: getpass2{,_stdin}(): Add macros
12:  0562e2e5 = 12:  3ff2a9de lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
13:  e8d08f96 = 13:  11310ac1 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
14:  8c4650c2 = 14:  c014e184 src/: Call passalloca() before a loop
15:  1d7a2ead = 15:  c649022b lib/pass/: Remove malloc(3)-based APIs, as they're unused
v9
  • Fix some diagnostics when calling passzero() in a loop, by reassigning the pointer as the return value, which lets the compiler know that the password isn't being used after zeroing.
$ git range-diff master gh/agetpass agetpass 
 1:  b02a8eb2 =  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
 2:  5deb458d =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 3:  bc41bf92 =  3:  bc41bf92 lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 4:  1c4bb330 =  4:  1c4bb330 lib/pass/agetpass.*: Redefine APIs as macros
 5:  3be94804 =  5:  3be94804 lib/pass/: readpass(): Add function
 6:  5185ddc7 =  6:  5185ddc7 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 7:  a75a8f83 =  7:  a75a8f83 lib/pass/: passzero(): Add function
 8:  345b50c5 =  8:  345b50c5 lib/pass/: Use passzero() instead of its pattern
 9:  e18d2c84 =  9:  e18d2c84 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
10:  15bc4454 = 10:  15bc4454 lib/pass/: passalloca(): Add macro
11:  191532d9 = 11:  191532d9 lib/pass/: getpass2{,_stdin}(): Add macros
12:  3ff2a9de = 12:  3ff2a9de lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
13:  11310ac1 = 13:  11310ac1 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
14:  c014e184 ! 14:  57930081 src/: Call passalloca() before a loop
    @@ Commit message
         buffer before the loop, and run getpass2() within the loop, which will
         reuse the buffer.
     
    +    Also, to avoid deallocator mismatches, use `pass = passzero(pass)` in
    +    those cases, so that the compiler knows that 'pass' has changed, and
    +    we're not using the password after zeroing it; we're only re-using its
    +    storage, which is fine.
    +
         Signed-off-by: Alejandro Colomar <[email protected]>
     
      ## src/gpasswd.c ##
    @@ src/gpasswd.c: static void change_passwd (struct group *gr)
                if (NULL == cp) {
                        MEMZERO(pass);
                        exit (1);
    +@@ src/gpasswd.c: static void change_passwd (struct group *gr)
    +                   break;
    +           }
    + 
    +-          passzero(cp);
    ++          cp = passzero(cp);
    +           MEMZERO(pass);
    + 
    +           if (retries + 1 < RETRIES) {
     
      ## src/passwd.c ##
     @@
    @@ src/passwd.c: static int new_password (const struct passwd *pw)
                        if (NULL == cp) {
                                MEMZERO(orig);
                                MEMZERO(pass);
    +@@ src/passwd.c: static int new_password (const struct passwd *pw)
    +                           warned = false;
    +                   }
    +                   ret = STRTCPY (pass, cp);
    +-                  passzero(cp);
    ++                  cp = passzero(cp);
    +                   if (ret == -1) {
    +                           (void) fputs (_("Password is too long.\n"), stderr);
    +                           MEMZERO(orig);
     @@ src/passwd.c: static int new_password (const struct passwd *pw)
                                warned = true;
                                continue;
    @@ src/passwd.c: static int new_password (const struct passwd *pw)
                        if (NULL == cp) {
                                MEMZERO(orig);
                                MEMZERO(pass);
    +                           return -1;
    +                   }
    +                   if (!streq(cp, pass)) {
    +-                          passzero(cp);
    ++                          cp = passzero(cp);
    +                           (void) fputs (_("They don't match; try again.\n"), stderr);
    +                   } else {
    +                           passzero(cp);
     
      ## src/sulogin.c ##
     @@
    @@ src/sulogin.c: main(int argc, char *argv[])
      
                /*
                 * XXX - can't enter single user mode if root password is
    +@@ src/sulogin.c: main(int argc, char *argv[])
    +           }
    + 
    +           done = valid(pass, &pwent);
    +-          passzero(pass);
    ++          pass = passzero(pass);
    + 
    +           if (!done) {    /* check encrypted passwords ... */
    +                   /* ... encrypted passwords did not match */
15:  c649022b = 15:  80070c1d lib/pass/: Remove malloc(3)-based APIs, as they're unused
v10
  • Use a type-safe macro ALLOCA() to wrap alloca(3).
$ git range-diff master gh/agetpass agetpass 
 1:  b02a8eb2 =  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
 2:  5deb458d =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 3:  bc41bf92 =  3:  bc41bf92 lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 4:  1c4bb330 =  4:  1c4bb330 lib/pass/agetpass.*: Redefine APIs as macros
 5:  3be94804 =  5:  3be94804 lib/pass/: readpass(): Add function
 6:  5185ddc7 =  6:  5185ddc7 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 7:  a75a8f83 =  7:  a75a8f83 lib/pass/: passzero(): Add function
 8:  345b50c5 =  8:  345b50c5 lib/pass/: Use passzero() instead of its pattern
 9:  e18d2c84 =  9:  e18d2c84 lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
 -:  -------- > 10:  2479cf9c lib/alloc/malloc.h: ALLOCA(): Add macro
10:  15bc4454 ! 11:  20874b1c lib/pass/: passalloca(): Add macro
    @@ lib/pass/passalloca.h (new)
     +
     +#include <config.h>
     +
    -+#include <alloca.h>
    -+
    ++#include "alloc/malloc.h"
     +#include "pass/limits.h"
     +
     +
    -+#define passalloca()   alloca(PASS_MAX + 2)
    ++#define passalloca()   ALLOCA(PASS_MAX + 2, char)
     +
     +
     +#endif  // include guard
11:  191532d9 = 12:  f4c3d8d0 lib/pass/: getpass2{,_stdin}(): Add macros
12:  3ff2a9de = 13:  a5131678 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
13:  11310ac1 = 14:  5c8501aa lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
14:  57930081 = 15:  5d062c5a src/: Call passalloca() before a loop
15:  80070c1d = 16:  b4c16dde lib/pass/: Remove malloc(3)-based APIs, as they're unused
v10b
  • Add missing restrict.
$ git range-diff master gh/agetpass agetpass 
 1:  b02a8eb2 =  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
 2:  5deb458d =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 3:  bc41bf92 =  3:  bc41bf92 lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 4:  1c4bb330 =  4:  1c4bb330 lib/pass/agetpass.*: Redefine APIs as macros
 5:  3be94804 !  5:  8b66e3fc lib/pass/: readpass(): Add function
    @@ lib/pass/readpass.c (new)
     +
     +// readpassphrase(3), but detect truncation, and memzero() on error.
     +char *
    -+readpass(char pass[PASS_MAX + 2], const char *prompt, int flags)
    ++readpass(char pass[restrict PASS_MAX + 2], const char *restrict prompt, int flags)
     +{
     +  size_t  len;
     +
    @@ lib/pass/readpass.h (new)
     +
     +
     +ATTR_MALLOC(memzero)
    -+char *readpass(char pass[PASS_MAX + 2], const char *prompt, int flags);
    ++char *readpass(char pass[restrict PASS_MAX + 2], const char *restrict prompt, int flags);
     +
     +
     +#endif  // include guard
 6:  5185ddc7 =  6:  aa3b2ab1 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 7:  a75a8f83 =  7:  f3fb8d7b lib/pass/: passzero(): Add function
 8:  345b50c5 !  8:  acbd3818 lib/pass/: Use passzero() instead of its pattern
    @@ lib/pass/readpass.h
      
     -ATTR_MALLOC(memzero)
     +ATTR_MALLOC(passzero)
    - char *readpass(char pass[PASS_MAX + 2], const char *prompt, int flags);
    + char *readpass(char pass[restrict PASS_MAX + 2], const char *restrict prompt, int flags);
      
      
 9:  e18d2c84 =  9:  32f6d31c lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
10:  2479cf9c = 10:  9cc1f964 lib/alloc/malloc.h: ALLOCA(): Add macro
11:  20874b1c = 11:  db8db161 lib/pass/: passalloca(): Add macro
12:  f4c3d8d0 = 12:  74b2127a lib/pass/: getpass2{,_stdin}(): Add macros
13:  a5131678 = 13:  c21e9393 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
14:  5c8501aa = 14:  38e73818 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
15:  5d062c5a = 15:  00f71453 src/: Call passalloca() before a loop
16:  b4c16dde = 16:  1510e8dc lib/pass/: Remove malloc(3)-based APIs, as they're unused
v11
  • Drop intermediate (and useless) commit.
$ git range-diff master gh/agetpass agetpass 
 1:  b02a8eb2 =  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
 2:  5deb458d =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 3:  bc41bf92 =  3:  bc41bf92 lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 4:  1c4bb330 =  4:  1c4bb330 lib/pass/agetpass.*: Redefine APIs as macros
 5:  8b66e3fc =  5:  8b66e3fc lib/pass/: readpass(): Add function
 6:  aa3b2ab1 =  6:  aa3b2ab1 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 7:  f3fb8d7b =  7:  f3fb8d7b lib/pass/: passzero(): Add function
 8:  acbd3818 =  8:  acbd3818 lib/pass/: Use passzero() instead of its pattern
 9:  32f6d31c <  -:  -------- lib/pass/: Rename agetpass_internal() => areadpass(), and move to separate file
10:  9cc1f964 =  9:  13d2547d lib/alloc/malloc.h: ALLOCA(): Add macro
11:  db8db161 ! 10:  b5fad4b8 lib/pass/: passalloca(): Add macro
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    +   pass/agetpass.c \
    +   pass/agetpass.h \
        pass/limits.h \
    -   pass/areadpass.c \
    -   pass/areadpass.h \
     +  pass/passalloca.c \
     +  pass/passalloca.h \
        pass/passzero.c \
12:  74b2127a ! 11:  b58fbc5e lib/pass/: getpass2{,_stdin}(): Add macros
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    +   pass/agetpass.c \
    +   pass/agetpass.h \
        pass/limits.h \
    -   pass/areadpass.c \
    -   pass/areadpass.h \
     +  pass/getpass2.c \
     +  pass/getpass2.h \
        pass/passalloca.c \
13:  c21e9393 = 12:  d4e74bf0 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
14:  38e73818 = 13:  0a5ca0e2 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
15:  00f71453 = 14:  dfefa3d2 src/: Call passalloca() before a loop
16:  1510e8dc ! 15:  04b32f09 lib/pass/: Remove malloc(3)-based APIs, as they're unused
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
     -  pass/agetpass.c \
     -  pass/agetpass.h \
        pass/limits.h \
    --  pass/areadpass.c \
    --  pass/areadpass.h \
        pass/getpass2.c \
        pass/getpass2.h \
    -   pass/passalloca.c \
     
      ## lib/pass/agetpass.c (deleted) ##
     @@
    @@ lib/pass/agetpass.c (deleted)
     -#include <config.h>
     -
     -#include "pass/agetpass.h"
    -
    - ## lib/pass/agetpass.h (deleted) ##
    -@@
    --// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
    --// SPDX-License-Identifier: BSD-3-Clause
    --
    --
    --#ifndef SHADOW_INCLUDE_LIB_AGETPASS_H_
    --#define SHADOW_INCLUDE_LIB_AGETPASS_H_
    --
    --
    --#include <config.h>
    --
    --#include <readpassphrase.h>
    --#include <stddef.h>
    --
    --#include "pass/areadpass.h"
    --
    --
    --// Similar to getpass(3), but free of its problems.
    --#define agetpass(prompt)  areadpass(prompt, RPP_REQUIRE_TTY)
    --#define agetpass_stdin()  areadpass(NULL, RPP_STDIN)
    --
    --
    --#endif  // include guard
    -
    - ## lib/pass/areadpass.c (deleted) ##
    -@@
    --// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
    --// SPDX-License-Identifier: BSD-3-Clause
    --
    --
    --#include <config.h>
    --
    --#include "pass/areadpass.h"
     -
     -#include <stddef.h>
     -#include <stdlib.h>
    @@ lib/pass/areadpass.c (deleted)
     -
     -
     -char *
    --areadpass(const char *prompt, int flags)
    +-agetpass_internal(const char *prompt, int flags)
     -{
     -  char    *pass;
     -  size_t  len;
    @@ lib/pass/areadpass.c (deleted)
     -
     -
     -void
    --erase_pass(char pass[PASS_MAX + 2])
    +-erase_pass(char *pass)
     -{
     -  free(passzero(pass));
     -}
     
    - ## lib/pass/areadpass.h (deleted) ##
    + ## lib/pass/agetpass.h (deleted) ##
     @@
    --// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
    --// SPDX-License-Identifier: BSD-3-Clause
    +-/*
    +- * SPDX-FileCopyrightText: 2022-2023, Alejandro Colomar <[email protected]>
    +- * SPDX-License-Identifier: BSD-3-Clause
    +- */
     -
     -
    --#ifndef SHADOW_INCLUDE_LIB_PASS_AREADPASS_H_
    --#define SHADOW_INCLUDE_LIB_PASS_AREADPASS_H_
    +-#ifndef SHADOW_INCLUDE_LIB_AGETPASS_H_
    +-#define SHADOW_INCLUDE_LIB_AGETPASS_H_
     -
     -
     -#include <config.h>
     -
    +-#include <readpassphrase.h>
    +-#include <stddef.h>
    +-
     -#include "attr.h"
    --#include "readpass.h"
     -
     -
    --void erase_pass(char pass[PASS_MAX + 2]);
    +-// Similar to getpass(3), but free of its problems.
    +-#define agetpass(prompt)  agetpass_internal(prompt, RPP_REQUIRE_TTY)
    +-#define agetpass_stdin()  agetpass_internal(NULL, RPP_STDIN)
    +-
    +-
    +-void erase_pass(char *pass);
     -ATTR_MALLOC(erase_pass)
    --char *areadpass(const char *prompt, int flags);
    +-char *agetpass_internal(const char *prompt, int flags);
     -
     -
     -#endif  // include guard
v12
  • Drop commit. Since we remove this file at the end of the PR, it doesn't make sense to clean it up in the middle.
$ git range-diff master gh/agetpass agetpass 
 1:  b02a8eb2 =  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
 2:  5deb458d =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 3:  bc41bf92 =  3:  bc41bf92 lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 4:  1c4bb330 <  -:  -------- lib/pass/agetpass.*: Redefine APIs as macros
 5:  8b66e3fc !  4:  a4468e67 lib/pass/: readpass(): Add function
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
     
      ## lib/pass/agetpass.c ##
     @@
    --/*
    -- * SPDX-FileCopyrightText:  2022, Alejandro Colomar <[email protected]>
    -- *
    -- * SPDX-License-Identifier:  BSD-3-Clause
    -- */
    -+// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
    -+// SPDX-License-Identifier: BSD-3-Clause
    - 
    - 
    - #include <config.h>
    - 
      #include "pass/agetpass.h"
      
    --#include <limits.h>
    + #include <limits.h>
     -#include <readpassphrase.h>
    -+#include <stddef.h>
      #include <stdlib.h>
    --#include <string.h>
    --
    --#ident "$Id$"
    + #include <string.h>
    + 
    + #ident "$Id$"
      
      #include "alloc/malloc.h"
     +#include "pass/readpass.h"
    @@ lib/pass/agetpass.c: agetpass_internal(const char *prompt, int flags)
        return NULL;
      }
      
    -+
    - void
    - erase_pass(char *pass)
    - {
     
      ## lib/pass/readpass.c (new) ##
     @@
 6:  aa3b2ab1 =  5:  13406d18 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
 7:  f3fb8d7b =  6:  57291ce8 lib/pass/: passzero(): Add function
 8:  acbd3818 !  7:  de697a16 lib/pass/: Use passzero() instead of its pattern
    @@ Commit message
     
      ## lib/pass/agetpass.c ##
     @@
    - #include <stdlib.h>
    + #ident "$Id$"
      
      #include "alloc/malloc.h"
     +#include "pass/passzero.h"
    @@ lib/pass/agetpass.c
     -#endif /* WITH_LIBBSD */
     -
      
    - char *
    + static char *
      agetpass_internal(const char *prompt, int flags)
    -@@ lib/pass/agetpass.c: fail:
    +@@ lib/pass/agetpass.c: agetpass_stdin()
      void
      erase_pass(char *pass)
      {
 9:  13d2547d =  8:  ad75a3b4 lib/alloc/malloc.h: ALLOCA(): Add macro
10:  b5fad4b8 =  9:  30604243 lib/pass/: passalloca(): Add macro
11:  b58fbc5e = 10:  1ab92613 lib/pass/: getpass2{,_stdin}(): Add macros
12:  d4e74bf0 = 11:  6bc8418a lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
13:  0a5ca0e2 = 12:  8d0a2f70 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
14:  dfefa3d2 = 13:  6f531866 src/: Call passalloca() before a loop
15:  04b32f09 ! 14:  ec6acbf2 lib/pass/: Remove malloc(3)-based APIs, as they're unused
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
     
      ## lib/pass/agetpass.c (deleted) ##
     @@
    --// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
    --// SPDX-License-Identifier: BSD-3-Clause
    +-/*
    +- * SPDX-FileCopyrightText:  2022, Alejandro Colomar <[email protected]>
    +- *
    +- * SPDX-License-Identifier:  BSD-3-Clause
    +- */
     -
     -
     -#include <config.h>
     -
     -#include "pass/agetpass.h"
     -
    --#include <stddef.h>
    +-#include <limits.h>
     -#include <stdlib.h>
    +-#include <string.h>
    +-
    +-#ident "$Id$"
     -
     -#include "alloc/malloc.h"
     -#include "pass/passzero.h"
     -#include "pass/readpass.h"
     -
     -
    --char *
    +-static char *
     -agetpass_internal(const char *prompt, int flags)
     -{
     -  char    *pass;
    @@ lib/pass/agetpass.c (deleted)
     -  return NULL;
     -}
     -
    +-// Similar to getpass(3), but free of its problems.
    +-char *
    +-agetpass(const char *prompt)
    +-{
    +-  return agetpass_internal(prompt, RPP_REQUIRE_TTY);
    +-}
    +-
    +-char *
    +-agetpass_stdin()
    +-{
    +-  return agetpass_internal(NULL, RPP_STDIN);
    +-}
     -
     -void
     -erase_pass(char *pass)
    @@ lib/pass/agetpass.h (deleted)
     -
     -#include <config.h>
     -
    --#include <readpassphrase.h>
    --#include <stddef.h>
    --
     -#include "attr.h"
     -
     -
    --// Similar to getpass(3), but free of its problems.
    --#define agetpass(prompt)  agetpass_internal(prompt, RPP_REQUIRE_TTY)
    --#define agetpass_stdin()  agetpass_internal(NULL, RPP_STDIN)
    --
    --
     -void erase_pass(char *pass);
     -ATTR_MALLOC(erase_pass)
    --char *agetpass_internal(const char *prompt, int flags);
    +-char *agetpass(const char *prompt);
    +-char *agetpass_stdin();
     -
     -
     -#endif  // include guard
v13
  • Drop commit. Since we remove this file at the end of the PR, it doesn't make sense to clean it up in the middle.
$ git range-diff master gh/agetpass agetpass 
 1:  b02a8eb2 =  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
 2:  5deb458d =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 3:  bc41bf92 <  -:  -------- lib/pass/, lib/, src/: Move agetpass() definition to within lib/pass/
 4:  a4468e67 !  3:  65e04c0b lib/pass/: readpass(): Add function
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    +   pam_defs.h \
    +   pam_pass.c \
        pam_pass_non_interactive.c \
    -   pass/agetpass.c \
    -   pass/agetpass.h \
     +  pass/readpass.c \
     +  pass/readpass.h \
        port.c \
        port.h \
        prefix_flag.c \
     
    - ## lib/pass/agetpass.c ##
    + ## lib/agetpass.c ##
     @@
    - #include "pass/agetpass.h"
    + #include "agetpass.h"
      
      #include <limits.h>
     -#include <readpassphrase.h>
    @@ lib/pass/agetpass.c
      
      #if WITH_LIBBSD == 0
      #include "freezero.h"
    -@@ lib/pass/agetpass.c: agetpass_internal(const char *prompt, int flags)
    +@@ lib/agetpass.c: agetpass_internal(const char *prompt, int flags)
        char    *pass;
        size_t  len;
      
    @@ lib/pass/agetpass.c: agetpass_internal(const char *prompt, int flags)
      }
      
     
    - ## lib/pass/readpass.c (new) ##
    + ## lib/readpass.c (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/pass/readpass.c (new)
     +  return NULL;
     +}
     
    - ## lib/pass/readpass.h (new) ##
    + ## lib/readpass.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
 5:  13406d18 !  4:  3fd46268 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    +   pam_defs.h \
    +   pam_pass.c \
        pam_pass_non_interactive.c \
    -   pass/agetpass.c \
    -   pass/agetpass.h \
     +  pass/limits.h \
        pass/readpass.c \
        pass/readpass.h \
    @@ lib/defines.h
     -
      #endif                            /* _DEFINES_H_ */
     
    - ## lib/pass/limits.h (new) ##
    + ## lib/limits.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/pass/limits.h (new)
     +
     +#endif  // include guard
     
    - ## lib/pass/readpass.c ##
    + ## lib/readpass.c ##
     @@
      #include <stddef.h>
      #include <string.h>
    @@ lib/pass/readpass.c
      
      
     
    - ## lib/pass/readpass.h ##
    + ## lib/readpass.h ##
     @@
      
      #include <config.h>
 6:  57291ce8 !  5:  aad144d9 lib/pass/: passzero(): Add function
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    -   pass/agetpass.c \
    -   pass/agetpass.h \
    +   pam_pass.c \
    +   pam_pass_non_interactive.c \
        pass/limits.h \
     +  pass/passzero.c \
     +  pass/passzero.h \
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        pass/readpass.h \
        port.c \
     
    - ## lib/pass/passzero.c (new) ##
    + ## lib/passzero.c (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/pass/passzero.c (new)
     +  return memzero(pass, PASS_MAX + 2);
     +}
     
    - ## lib/pass/passzero.h (new) ##
    + ## lib/passzero.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
 7:  de697a16 !  6:  228fffa7 lib/pass/: Use passzero() instead of its pattern
    @@ Commit message
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    - ## lib/pass/agetpass.c ##
    + ## lib/agetpass.c ##
     @@
      #ident "$Id$"
      
    @@ lib/pass/agetpass.c
      
      static char *
      agetpass_internal(const char *prompt, int flags)
    -@@ lib/pass/agetpass.c: agetpass_stdin()
    +@@ lib/agetpass.c: agetpass_stdin()
      void
      erase_pass(char *pass)
      {
    @@ lib/pass/agetpass.c: agetpass_stdin()
     +  free(passzero(pass));
      }
     
    - ## lib/pass/readpass.h ##
    + ## lib/readpass.h ##
     @@
      
      #include "attr.h"
 8:  ad75a3b4 =  7:  090decf9 lib/alloc/malloc.h: ALLOCA(): Add macro
 9:  30604243 !  8:  e064081a lib/pass/: passalloca(): Add macro
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    -   pass/agetpass.c \
    -   pass/agetpass.h \
    +   pam_pass.c \
    +   pam_pass_non_interactive.c \
        pass/limits.h \
     +  pass/passalloca.c \
     +  pass/passalloca.h \
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        pass/passzero.h \
        pass/readpass.c \
     
    - ## lib/pass/passalloca.c (new) ##
    + ## lib/passalloca.c (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/pass/passalloca.c (new)
     +
     +#include "pass/passalloca.h"
     
    - ## lib/pass/passalloca.h (new) ##
    + ## lib/passalloca.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
10:  1ab92613 !  9:  cd9d3fbb lib/pass/: getpass2{,_stdin}(): Add macros
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    -   pass/agetpass.c \
    -   pass/agetpass.h \
    +   pam_pass.c \
    +   pam_pass_non_interactive.c \
        pass/limits.h \
     +  pass/getpass2.c \
     +  pass/getpass2.h \
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        pass/passalloca.h \
        pass/passzero.c \
     
    - ## lib/pass/getpass2.c (new) ##
    + ## lib/getpass2.c (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/pass/getpass2.c (new)
     +
     +#include "pass/getpass2.h"
     
    - ## lib/pass/getpass2.h (new) ##
    + ## lib/getpass2.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
11:  6bc8418a ! 10:  48e7de83 lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
    @@ Commit message
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    - ## lib/pass/getpass2.h ##
    + ## lib/getpass2.h ##
     @@
      #include <readpassphrase.h>
      #include <stddef.h>
12:  8d0a2f70 ! 11:  38a8408b lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
    @@ lib/pwauth.c
      #include <sys/types.h>
      #include <unistd.h>
      
    --#include "pass/agetpass.h"
    +-#include "agetpass.h"
      #include "defines.h"
     +#include "pass/getpass2.h"
     +#include "pass/passzero.h"
    @@ lib/pwauth.c: int pw_auth (const char *cipher,
      }
     
      ## src/gpasswd.c ##
    +@@
    + #include <stdio.h>
    + #include <sys/types.h>
    + 
    +-#include "agetpass.h"
    + #include "alloc/x/xmalloc.h"
    + #include "attr.h"
    + #include "defines.h"
     @@
      #include "exitcodes.h"
      #include "groupio.h"
      #include "nscd.h"
    --#include "pass/agetpass.h"
     +#include "pass/getpass2.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
    @@ src/gpasswd.c: static void change_passwd (struct group *gr)
     
      ## src/newgrp.c ##
     @@
    + #include <stdio.h>
    + #include <sys/types.h>
    + 
    +-#include "agetpass.h"
    + #include "alloc/x/xmalloc.h"
    + #include "chkname.h"
    + #include "defines.h"
      /*@-exitarg@*/
      #include "exitcodes.h"
      #include "getdef.h"
    --#include "pass/agetpass.h"
     +#include "pass/getpass2.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
    @@ src/newgrp.c: static void check_perms (const struct group *grp,
     
      ## src/passwd.c ##
     @@
    + #include <sys/types.h>
    + #include <time.h>
    + 
    +-#include "agetpass.h"
    + #include "atoi/a2i/a2s.h"
    + #include "chkname.h"
      #include "defines.h"
      #include "getdef.h"
      #include "nscd.h"
    --#include "pass/agetpass.h"
     +#include "pass/getpass2.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
    @@ src/passwd.c: main(int argc, char **argv)
     
      ## src/sulogin.c ##
     @@
    + #include <sys/ioctl.h>
    + #include <sys/types.h>
    + 
    +-#include "agetpass.h"
      #include "attr.h"
      #include "defines.h"
      #include "getdef.h"
    --#include "pass/agetpass.h"
     +#include "pass/getpass2.h"
     +#include "pass/passzero.h"
      #include "prototypes.h"
13:  6f531866 = 12:  822aa6dd src/: Call passalloca() before a loop
14:  ec6acbf2 ! 13:  28252927 lib/pass/: Remove malloc(3)-based APIs, as they're unused
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    -   pam_defs.h \
    -   pam_pass.c \
    -   pam_pass_non_interactive.c \
    --  pass/agetpass.c \
    --  pass/agetpass.h \
    -   pass/limits.h \
    -   pass/getpass2.c \
    -   pass/getpass2.h \
    +   adds.c \
    +   adds.h \
    +   age.c \
    +-  agetpass.c \
    +-  agetpass.h \
    +   alloc/calloc.c \
    +   alloc/calloc.h \
    +   alloc/malloc.c \
     
    - ## lib/pass/agetpass.c (deleted) ##
    + ## lib/agetpass.c (deleted) ##
     @@
     -/*
     - * SPDX-FileCopyrightText:  2022, Alejandro Colomar <[email protected]>
    @@ lib/pass/agetpass.c (deleted)
     -
     -#include <config.h>
     -
    --#include "pass/agetpass.h"
    +-#include "agetpass.h"
     -
     -#include <limits.h>
     -#include <stdlib.h>
    @@ lib/pass/agetpass.c (deleted)
     -  free(passzero(pass));
     -}
     
    - ## lib/pass/agetpass.h (deleted) ##
    + ## lib/agetpass.h (deleted) ##
     @@
     -/*
     - * SPDX-FileCopyrightText: 2022-2023, Alejandro Colomar <[email protected]>
    @@ lib/pass/agetpass.h (deleted)
     -#include <config.h>
     -
     -#include "attr.h"
    +-#include "defines.h"
     -
     -
     -void erase_pass(char *pass);
v13b
  • Restore the lib/pass/ directory, which was accidentally dropped in the rebase in v13.
$ git range-diff master gh/agetpass agetpass 
 1:  b02a8eb2 =  1:  b02a8eb2 src/sulogin.c: main(): Add local variable
 2:  5deb458d =  2:  5deb458d lib/agetpass.h: Replace documentation by one-line comment
 3:  65e04c0b !  3:  f4e6e56d lib/pass/: readpass(): Add function
    @@ lib/agetpass.c: agetpass_internal(const char *prompt, int flags)
      }
      
     
    - ## lib/readpass.c (new) ##
    + ## lib/pass/readpass.c (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/readpass.c (new)
     +  return NULL;
     +}
     
    - ## lib/readpass.h (new) ##
    + ## lib/pass/readpass.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
 4:  3fd46268 !  4:  956e2468 lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
    @@ lib/defines.h
     -
      #endif                            /* _DEFINES_H_ */
     
    - ## lib/limits.h (new) ##
    + ## lib/pass/limits.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/limits.h (new)
     +
     +#endif  // include guard
     
    - ## lib/readpass.c ##
    + ## lib/pass/readpass.c ##
     @@
      #include <stddef.h>
      #include <string.h>
    @@ lib/readpass.c
      
      
     
    - ## lib/readpass.h ##
    + ## lib/pass/readpass.h ##
     @@
      
      #include <config.h>
 5:  aad144d9 !  5:  895dd80f lib/pass/: passzero(): Add function
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        pass/readpass.h \
        port.c \
     
    - ## lib/passzero.c (new) ##
    + ## lib/pass/passzero.c (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/passzero.c (new)
     +  return memzero(pass, PASS_MAX + 2);
     +}
     
    - ## lib/passzero.h (new) ##
    + ## lib/pass/passzero.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
 6:  228fffa7 !  6:  c397766a lib/pass/: Use passzero() instead of its pattern
    @@ lib/agetpass.c: agetpass_stdin()
     +  free(passzero(pass));
      }
     
    - ## lib/readpass.h ##
    + ## lib/pass/readpass.h ##
     @@
      
      #include "attr.h"
 7:  090decf9 =  7:  71674f6b lib/alloc/malloc.h: ALLOCA(): Add macro
 8:  e064081a !  8:  bc8d13f5 lib/pass/: passalloca(): Add macro
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        pass/passzero.h \
        pass/readpass.c \
     
    - ## lib/passalloca.c (new) ##
    + ## lib/pass/passalloca.c (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/passalloca.c (new)
     +
     +#include "pass/passalloca.h"
     
    - ## lib/passalloca.h (new) ##
    + ## lib/pass/passalloca.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
 9:  cd9d3fbb !  9:  4af989dd lib/pass/: getpass2{,_stdin}(): Add macros
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        pass/passalloca.h \
        pass/passzero.c \
     
    - ## lib/getpass2.c (new) ##
    + ## lib/pass/getpass2.c (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
    @@ lib/getpass2.c (new)
     +
     +#include "pass/getpass2.h"
     
    - ## lib/getpass2.h (new) ##
    + ## lib/pass/getpass2.h (new) ##
     @@
     +// SPDX-FileCopyrightText: 2022-2025, Alejandro Colomar <[email protected]>
     +// SPDX-License-Identifier: BSD-3-Clause
10:  48e7de83 ! 10:  d8ac814a lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these APIs
    @@ Commit message
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    - ## lib/getpass2.h ##
    + ## lib/pass/getpass2.h ##
     @@
      #include <readpassphrase.h>
      #include <stddef.h>
11:  38a8408b = 11:  6036c9a0 lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
12:  822aa6dd = 12:  f9caefc3 src/: Call passalloca() before a loop
13:  28252927 ! 13:  2a5cf770 lib/pass/: Remove malloc(3)-based APIs, as they're unused
    @@ Metadata
     Author: Alejandro Colomar <[email protected]>
     
      ## Commit message ##
    -    lib/pass/: Remove malloc(3)-based APIs, as they're unused
    +    lib/agetpass.*: Remove malloc(3)-based APIs, as they're unused
     
         In the last commit, we replaced all of these calls by alloca(3)-based
         variants.

Comparison against v12, as a sanity check:

alx@devuan:/srv/alx/src/shadow/shadow/master$ git diff ec6acbf2..2a5cf770
alx@devuan:/srv/alx/src/shadow/shadow/master$

@alejandro-colomar alejandro-colomar force-pushed the agetpass branch 4 times, most recently from cafa934 to 1154d32 Compare January 19, 2025 23:55
@alejandro-colomar alejandro-colomar marked this pull request as ready for review January 20, 2025 00:01
@alejandro-colomar alejandro-colomar force-pushed the agetpass branch 4 times, most recently from f6aeb0a to b7c072b Compare January 20, 2025 00:24
alejandro-colomar
alejandro-colomar commented Jan 20, 2025
View reviewed changes
lib/agetpass.h Outdated Show resolved Hide resolved
@alejandro-colomar alejandro-colomar marked this pull request as draft January 22, 2025 01:37
@alejandro-colomar

This comment was marked as outdated.

Sign in to view
@alejandro-colomar
Copy link
Collaborator Author

I've rewritten the patches from scratch as v4.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 22, 2025
View reviewed changes
src/passwd.c Fixed Show resolved Hide resolved
src/passwd.c Fixed Show fixed Hide fixed
@alejandro-colomar alejandro-colomar force-pushed the agetpass branch 2 times, most recently from 57b8ac5 to c7ea351 Compare January 22, 2025 15:41
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 22, 2025
View reviewed changes
lib/pwauth.c Fixed Show fixed Hide fixed
@alejandro-colomar alejandro-colomar marked this pull request as ready for review January 23, 2025 00:25
@alejandro-colomar alejandro-colomar force-pushed the agetpass branch 2 times, most recently from fb69c8d to d6ceb22 Compare January 24, 2025 15:05
@alejandro-colomar
src/sulogin.c: main(): Add local variable
b02a8eb 
This simplifies the agetpass() call into a single line.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/agetpass.h: Replace documentation by one-line comment
5deb458 
The lengthy documentation is rather obvious, and only clutters the
source file.  It will still be reachable in the git history for those
interested.  Instead, just say that this function is basically
getpass(3) done right.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar alejandro-colomar force-pushed the agetpass branch 2 times, most recently from 9c49413 to 1d7a2ea Compare January 24, 2025 23:11
@alejandro-colomar alejandro-colomar force-pushed the agetpass branch 7 times, most recently from ec6acbf to 2825292 Compare January 26, 2025 21:02
@alejandro-colomar
lib/pass/: readpass(): Add function
f4e6e56 
readpassphrase(3) is hard to use correctly.  Wrap correct usage of
readpassphrase in this API.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/pass/limits.h, lib/: PASS_MAX: Move definition to under lib/pass/
956e246 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/pass/: passzero(): Add function
895dd80 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/pass/: Use passzero() instead of its pattern
c397766 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/alloc/malloc.h: ALLOCA(): Add macro
71674f6 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/pass/: passalloca(): Add macro
bc8d13f 
This macro will allow using alloca(3) memory in these APIs.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/pass/: getpass2{,_stdin}(): Add macros
4af989d 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/pass/: getpassa{,_stdin}(): Add alloca(3)-based variants of these…
d8ac814 
… APIs

These APIs will minimize the visibility of passwords, by not using the
heap.  The stack should have enough space for PASS_MAX+2 allocations, so
this should be safe.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/, src/: Use getpassa()/passzero() instead of agetpass()/erase_pass()
6036c9a 
And getpassa_stdin() instead of agetpass_stdin().

Now all passwords live in the stack, and are never copied into the heap.

This introduces a subtle issue: while it's fine to call malloc(3) in a
loop, it is dangerous to call alloca(3) in a loop (since there's no way
to free that memory).  The next commit will fix that.  I've addressed it
in a separate commit, for readability.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/: Call passalloca() before a loop
f9caefc 
Calling passalloca() (which is a wrapper around alloca(3)) in a loop is
dangerous, as it can trigger a stack overflow.  Instead, allocate the
buffer before the loop, and run getpass2() within the loop, which will
reuse the buffer.

Also, to avoid deallocator mismatches, use `pass = passzero(pass)` in
those cases, so that the compiler knows that 'pass' has changed, and
we're not using the password after zeroing it; we're only re-using its
storage, which is fine.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/agetpass.*: Remove malloc(3)-based APIs, as they're unused
2a5cf77 
In the last commit, we replaced all of these calls by alloca(3)-based
variants.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
Copy link
Collaborator Author

@jubalh Do you know why the opensuse CI is failing? It's something about the package manager, it seems.

@ikerexxe
Copy link
Collaborator

Thank you for your work on these patches. I've started reviewing them and I have a few questions to help me better understand the overall goal. I appreciate you've been working to improve the project and introduce new APIs for things like password handling.

While I understand the initial need for these APIs, I'm finding the frequent modifications a bit challenging to follow. Can you provide some more context around the long-term vision for these APIs? Understanding the ultimate goal would help assess the individual changes and provide more meaningful feedback.

@alejandro-colomar
Copy link
Collaborator Author

Thank you for your work on these patches. I've started reviewing them and I have a few questions to help me better understand the overall goal. I appreciate you've been working to improve the project and introduce new APIs for things like password handling.

While I understand the initial need for these APIs, I'm finding the frequent modifications a bit challenging to follow. Can you provide some more context around the long-term vision for these APIs? Understanding the ultimate goal would help assess the individual changes and provide more meaningful feedback.

The goal of the password APIs is that

  • We guarantee that all passwords are zeroed before the memory is freed, via the [[gnu::malloc(passzero)]] attribute.
  • The password never lives in the heap. That makes sure that attacks that read the stack are unable to read the passwords.
  • There are no buffer overflows nor truncation when handling passwords. All buffers should be exactly PASS_MAX+2 bytes long.

Currently, we read the passwords with agetpass(), which provides the first guarantee and the third, but not the second.
We make sure to freezero(3) the result of agetpass() as soon as possible, and copy it into a local array, so that it lives in the heap for a very short time.

This has another problem: there might be truncation or buffer overflows if the buffers don't match in size. The solution to that is using lib/pass/ APIs when handling those copies too.

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Jan 27, 2025 •
edited
Loading

Here's the list of APIs that I want to have:

// There is also a limit in PAM (PAM_MAX_RESP_SIZE), currently set to 512.
#ifndef  PASS_MAX
# define PASS_MAX  (BUFSIZ - 1)
#endif


// Similar to getpass(3), but free of its problems, and get the buffer in $1.
#define getpass2(buf, prompt)  readpass(buf, prompt, RPP_REQUIRE_TTY)
#define getpass2_stdin(buf)    readpass(buf, NULL, RPP_STDIN)

// Similar to getpass(3), but free of its problems, and using alloca(3).
#define getpassa(prompt)  getpass2(passalloca(), prompt)
#define getpassa_stdin()  getpass2_stdin(passalloca())

#define passalloca()    ALLOCA(1, pass_t)
#define passcalloca()   passdupa(&(pass_t){""})
#define passdupa(pass)  passcpy(passalloca(), pass)


typedef typeof(char [PASS_MAX + 2])  pass_t;


ATTR_MALLOC(passzero)
ATTR_STRING(2)
pass_t *passcpy(pass_t *restrict dst, const pass_t *restrict src);

ATTR_STRING(1)
pass_t *passzero(pass_t *pass);

ATTR_MALLOC(passzero)
ATTR_STRING(2)
pass_t *readpass(pass_t *restrict pass, const char *restrict prompt, int flags);

of which, readpass() is only a helper function, but the rest should be used in actual code.

@alejandro-colomar
Copy link
Collaborator Author

Here's the list of APIs that I want to have:

// There is also a limit in PAM (PAM_MAX_RESP_SIZE), currently set to 512.
#ifndef  PASS_MAX
# define PASS_MAX  (BUFSIZ - 1)
#endif


// Similar to getpass(3), but free of its problems, and get the buffer in $1.
#define getpass2(buf, prompt)  readpass(buf, prompt, RPP_REQUIRE_TTY)
#define getpass2_stdin(buf)    readpass(buf, NULL, RPP_STDIN)

// Similar to getpass(3), but free of its problems, and using alloca(3).
#define getpassa(prompt)  getpass2(passalloca(), prompt)
#define getpassa_stdin()  getpass2_stdin(passalloca())

#define passalloca()    ALLOCA(1, pass_t)
#define passcalloca()   passdupa(&(pass_t){""})
#define passdupa(pass)  passcpy(passalloca(), pass)


typedef typeof(char [PASS_MAX + 2])  pass_t;


ATTR_MALLOC(passzero)
ATTR_STRING(2)
pass_t *passcpy(pass_t *restrict dst, const pass_t *restrict src);

ATTR_STRING(1)
pass_t *passzero(pass_t *pass);

ATTR_MALLOC(passzero)
ATTR_STRING(2)
pass_t *readpass(pass_t *restrict pass, const char *restrict prompt, int flags);

of which, readpass() is only a helper function, but the rest should be used in actual code.

Hmmm, since most of them are one-liners, and only a couple of functions, maybe I can keep a single pair of .c/.h files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uecker uecker uecker left review comments

@ikerexxe ikerexxe Awaiting requested review from ikerexxe

@hallyn hallyn Awaiting requested review from hallyn

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alejandro-colomar @ikerexxe @uecker